Add fuzzing targets for snappy-java#721
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the snappy-java library by integrating fuzzing targets. These targets are designed to continuously test various compression and decompression functionalities, including raw APIs, framed streams, bit shuffling, and CRC32C calculations, ensuring robustness against unexpected inputs and potential vulnerabilities. The addition of a CIFuzz workflow automates this testing, providing an ongoing layer of security assurance. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces fuzzing targets for snappy-java, which is a valuable addition for enhancing the security and robustness of the library. However, the current implementation of the fuzzers has some critical issues. The most significant problem is that exceptions are caught and swallowed in many places, which prevents the fuzzing engine from detecting and reporting crashes. This undermines the primary purpose of fuzzing. These empty catch blocks should be updated to rethrow exceptions. Additionally, there are several instances of resource leaks where streams are not properly closed. Using try-with-resources would be the ideal solution for managing these resources. I have provided specific comments and code suggestions to address these points.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces fuzzing targets for snappy-java, which is a great step for improving the project's security and robustness. The new fuzzers cover a wide range of APIs. My review focuses on improving the code style, maintainability, and correctness of the new fuzzer classes. I've pointed out some areas with code duplication, non-standard styling, and a couple of issues in exception handling within the fuzzers that could lead to false positives. Overall, these are valuable additions to the project.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several fuzzing targets for snappy-java, which is a great step towards improving the library's robustness and security. The fuzzers cover a wide range of APIs. However, I've found a critical issue in SnappyCombinedFuzzer.java where exceptions are being swallowed in many of the test cases. This practice undermines the effectiveness of fuzzing, as it can hide crashes and other unexpected behavior that the fuzzer is designed to find. My review comments focus on fixing these issues to ensure the fuzzers can effectively report bugs.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces three new fuzzing targets for snappy-java using Jazzer, covering the BitShuffle, SnappyStream, and a combination of other Snappy APIs. The changes are a good step towards improving the security and robustness of the library through continuous fuzzing.
My review focuses on improving the correctness and maintainability of the new fuzzers. The main points are:
- A critical issue in
SnappyCombinedFuzzerwhere theFuzzedDataProvideris used incorrectly, causing large parts of the fuzzer to be ineffective. - Opportunities to improve the
SnappyCombinedFuzzerby adding assertions to verify logic, not just find crashes. - Suggestions to refactor duplicated code in
BitShuffleFuzzerandSnappyCombinedFuzzerto enhance maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several fuzzing targets for snappy-java, which is a great step towards improving the security and robustness of the library. The fuzzers cover a wide range of APIs, from raw compression to framed streams and bit shuffling.
My review focuses on improving the effectiveness of the new fuzzers. I've identified a recurring issue in SnappyCombinedFuzzer.java where the FuzzedDataProvider is exhausted prematurely, causing some test cases to run with empty data. Additionally, several fuzzing routines are missing correctness assertions, which limits them to finding only crashes rather than silent data corruption bugs. I've provided detailed suggestions to fix these issues in each affected test method to ensure the fuzzers can find a wider range of problems.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces fuzzing targets for snappy-java, which is a great step towards improving the security and robustness of the library. The new fuzzers cover a wide range of APIs, including the raw API, framed streams, BitShuffle, and more.
My review focuses on improving the effectiveness and maintainability of the new fuzzing code. I've identified a few areas with opportunities for improvement:
- In
SnappyCombinedFuzzer, some fuzzing logic doesn't validate the results of the operations, which limits its effectiveness to only finding crashes. - There's also an incomplete test case in
SnappyCombinedFuzzerthat should be either completed or removed. - In
BitShuffleFuzzer, there is significant code duplication that could be refactored to improve maintainability.
Overall, this is a valuable addition to the project. Addressing these points will make the fuzzers even more effective.
| static void fuzzBitshuffleInts(int[] original) { | ||
| int[] result; | ||
|
|
||
| try { | ||
| byte[] shuffledByteArray = BitShuffle.shuffle(original); | ||
| byte[] compressed = Snappy.compress(shuffledByteArray); | ||
| byte[] uncompressed = Snappy.uncompress(compressed); | ||
| result = BitShuffle.unshuffleIntArray(uncompressed); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
|
|
||
| if (!Arrays.equals(original, result)) { | ||
| throw new IllegalStateException("Original and uncompressed data are different"); | ||
| } | ||
| } | ||
|
|
||
| static void fuzzBitshuffleLongs(long[] original) { | ||
| long[] result; | ||
|
|
||
| try { | ||
| byte[] shuffledByteArray = BitShuffle.shuffle(original); | ||
| byte[] compressed = Snappy.compress(shuffledByteArray); | ||
| byte[] uncompressed = Snappy.uncompress(compressed); | ||
| result = BitShuffle.unshuffleLongArray(uncompressed); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
|
|
||
| if (!Arrays.equals(original, result)) { | ||
| throw new IllegalStateException("Original and uncompressed data are different"); | ||
| } | ||
| } | ||
|
|
||
| static void fuzzBitshuffleShorts(short[] original) { | ||
| short[] result; | ||
|
|
||
| try { | ||
| byte[] shuffledByteArray = BitShuffle.shuffle(original); | ||
| byte[] compressed = Snappy.compress(shuffledByteArray); | ||
| byte[] uncompressed = Snappy.uncompress(compressed); | ||
| result = BitShuffle.unshuffleShortArray(uncompressed); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
|
|
||
| if (!Arrays.equals(original, result)) { | ||
| throw new IllegalStateException("Original and uncompressed data are different"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The methods fuzzBitshuffleInts, fuzzBitshuffleLongs, and fuzzBitshuffleShorts are nearly identical. This code duplication can be avoided by refactoring them into a single private generic method. This would improve the maintainability of the code.
| private static void testCrc32C(FuzzedDataProvider data) { | ||
| byte[] input = data.consumeBytes(data.consumeInt(0, 4096)); | ||
| byte[] chunk1 = data.consumeBytes(50); | ||
| byte[] chunk2 = data.consumeBytes(50); | ||
|
|
||
| PureJavaCrc32C crc = new PureJavaCrc32C(); | ||
| crc.update(input, 0, input.length); | ||
| long value = crc.getValue(); | ||
|
|
||
| int intValue = crc.getIntegerValue(); | ||
|
|
||
| crc.reset(); | ||
| crc.update(input, 0, input.length); | ||
| long value2 = crc.getValue(); | ||
| if (value != value2) { | ||
| throw new IllegalStateException("CRC32C reset produced different value"); | ||
| } | ||
|
|
||
| PureJavaCrc32C crcChunked = new PureJavaCrc32C(); | ||
| for (int i = 0; i < Math.min(input.length, 1000); i++) { | ||
| crcChunked.update(input[i] & 0xFF); | ||
| } | ||
|
|
||
| if (chunk1.length > 0 && chunk2.length > 0) { | ||
| PureJavaCrc32C crc1 = new PureJavaCrc32C(); | ||
| crc1.update(input, 0, input.length); | ||
|
|
||
| PureJavaCrc32C crc2 = new PureJavaCrc32C(); | ||
| crc2.update(chunk1, 0, chunk1.length); | ||
| crc2.update(chunk2, 0, chunk2.length); | ||
| } | ||
|
|
||
| PureJavaCrc32C crcEmpty = new PureJavaCrc32C(); | ||
| crcEmpty.update(new byte[0], 0, 0); | ||
| long emptyValue = crcEmpty.getValue(); | ||
|
|
||
| if (input.length > 0) { | ||
| PureJavaCrc32C crcSingle = new PureJavaCrc32C(); | ||
| crcSingle.update(input[0] & 0xFF); | ||
| } | ||
|
|
||
| if (input.length > 4) { | ||
| PureJavaCrc32C crcOffset = new PureJavaCrc32C(); | ||
| crcOffset.update(input, 1, input.length - 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
In testCrc32C, several computed CRC values are not used or validated (e.g., intValue, crcChunked, crc1, crc2, emptyValue, crcSingle, crcOffset). This reduces the effectiveness of the fuzzer, as it will only detect crashes but not incorrect CRC calculations. Consider adding assertions to verify the behavior of the CRC32C implementation, for example by comparing the results of different ways of computing the CRC for the same data.
| runFuzz(() -> { | ||
| byte[] input = data.consumeBytes(data.consumeInt(0, 4096)); | ||
| ByteBuffer directSrc = ByteBuffer.allocateDirect(input.length); | ||
| directSrc.put(input); | ||
| directSrc.flip(); | ||
|
|
||
| ByteBuffer directDst = ByteBuffer.allocateDirect(Snappy.maxCompressedLength(input.length)); | ||
| Snappy.compress(directSrc, directDst); | ||
| }); |
There was a problem hiding this comment.
This fuzzing block only calls Snappy.compress and doesn't validate the output or perform a round-trip test. The previous block in testByteBuffer (lines 348-369) already covers the compress/uncompress roundtrip for ByteBuffer. This block is redundant and incomplete. Please either remove it or add validation to make it a meaningful test.
|
/gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces fuzzing targets for snappy-java, which is a great step towards improving the library's robustness and security. The fuzzers cover a wide range of APIs, including the raw API, framed streams, and BitShuffle. My review focuses on improving the effectiveness of some tests and reducing code duplication in the new fuzzer files for better maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several fuzzing targets for snappy-java using Jazzer, which is a great step towards improving the security and robustness of the library. The new fuzzers cover the raw API, framed streams, BitShuffle, and other utilities.
My review has found a critical issue in BitShuffleFuzzer that would cause a ClassCastException, preventing it from working as intended. I've also pointed out a few minor opportunities for code cleanup by removing unused imports and an unused interface.
Once the critical issue is addressed, these fuzzers will be a valuable addition to the project's test suite.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces fuzzing for snappy-java by adding three new fuzzers for various components like BitShuffle, SnappyOutputStream, and a combined fuzzer for multiple APIs. The implementation is robust, performing round-trip checks to ensure data integrity after compression and decompression cycles. The code is well-structured. I have one minor suggestion in SnappyCombinedFuzzer to improve code clarity.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces fuzzing targets for snappy-java using CIFuzz/OSS-Fuzz, which is a great step towards improving the security and robustness of the library. The new fuzzers cover a wide range of APIs, including the raw API, framed streams, block streams, and the BitShuffle algorithm. The code is well-structured and the fuzzing logic is sound. I have a couple of suggestions to improve test coverage and code maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces fuzzing targets for snappy-java, which is a great step towards improving the security and robustness of the library. The new fuzzers cover a wide range of APIs, including the raw API, framed streams, BitShuffle, and more. The code is well-structured. I've found a few areas for improvement to make the fuzzers even more effective, such as removing a duplicated test case and using fuzzed values for some hardcoded sizes to increase coverage. Overall, this is a valuable addition to the project.
| runFuzz(() -> { | ||
| int inputLength = data.consumeInt(0, 4096); | ||
| int maxLen = Snappy.maxCompressedLength(inputLength); | ||
| if (maxLen < inputLength) { | ||
| throw new IllegalStateException("maxCompressedLength too small"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This test for Snappy.maxCompressedLength is redundant. An identical test exists in the testUtil method (lines 247-253). To avoid code duplication, I suggest removing this block.
|
|
||
| runFuzz(() -> { | ||
| try (SnappyFramedInputStream invalidIn = new SnappyFramedInputStream( | ||
| new ByteArrayInputStream(data.consumeBytes(100)))) { |
There was a problem hiding this comment.
The size of the byte array for testing with invalid framed stream data is hardcoded to 100. To improve fuzzing coverage, consider using a variable size determined by the fuzzer input.
| new ByteArrayInputStream(data.consumeBytes(100)))) { | |
| new ByteArrayInputStream(data.consumeBytes(data.consumeInt(0, 1024))))) { |
| }); | ||
|
|
||
| runFuzz(() -> { | ||
| try (SnappyInputStream in = new SnappyInputStream(new ByteArrayInputStream(data.consumeBytes(100)))) { |
There was a problem hiding this comment.
The size of the byte array for testing with invalid block stream data is hardcoded to 100. To improve fuzzing coverage, consider using a variable size determined by the fuzzer input.
| try (SnappyInputStream in = new SnappyInputStream(new ByteArrayInputStream(data.consumeBytes(100)))) { | |
| try (SnappyInputStream in = new SnappyInputStream(new ByteArrayInputStream(data.consumeBytes(data.consumeInt(0, 1024))))) { |
| byte[] uncompressed; | ||
| try (SnappyInputStream snappyIn = new SnappyInputStream(new ByteArrayInputStream(compressed))) { | ||
| ByteArrayOutputStream out = new ByteArrayOutputStream(); | ||
| byte[] buf = new byte[4096]; |
There was a problem hiding this comment.
The buffer size for reading from SnappyInputStream is hardcoded to 4096. To test different read patterns and potentially uncover more bugs, it would be beneficial to use a fuzzed buffer size. You could define a bufferSize variable at the start of fuzzerTestOneInput by consuming an integer from the FuzzedDataProvider (e.g., int bufferSize = data.consumeInt(1, 8192);), and use it here.
| byte[] buf = new byte[4096]; | |
| byte[] buf = new byte[bufferSize]; |
…nappy-java into adding-new-fuzzers
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces fuzzing targets for snappy-java, which is an excellent initiative for enhancing the library's security and robustness. The fuzzers cover a broad range of APIs, from raw compression to framed streams and BitShuffle. The implementation is generally solid. My review includes a few suggestions for SnappyCombinedFuzzer.java to improve the tests against malformed data. By handling expected IOExceptions, we can prevent the fuzzing engine from reporting false positives, making the fuzzing results more reliable.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several fuzzing targets for snappy-java to facilitate continuous security testing. The fuzzers cover a broad range of APIs, including the raw API, framed streams, block streams, BitShuffle, and ByteBuffer APIs. The implementation is well-structured, notably using a selector in SnappyCombinedFuzzer to target different functionalities. I have a couple of suggestions to enhance the fuzzing efficiency by making the tests more granular, which should help the fuzzing engine explore different code paths more effectively.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces three new fuzzer targets: BitShuffleFuzzer, SnappyCombinedFuzzer, and SnappyStreamFuzzer, along with a helper functional interface FuzzBlock. These fuzzers significantly enhance the continuous security testing for snappy-java by covering a broad range of APIs, including raw compression/decompression, framed streams, CRC32C, block streams, utility methods, Hadoop streams, ByteBuffer operations, and BitShuffle algorithms. The implementation is well-structured, utilizing FuzzedDataProvider effectively to generate diverse inputs and employing consistent error handling patterns suitable for fuzzing harnesses. The addition of these fuzzers is a valuable contribution to the project's robustness.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces fuzzing targets for snappy-java, enhancing continuous security testing. The fuzzers cover a broad range of APIs, including the raw API, framed streams, and the BitShuffle algorithm. My review identifies a few areas for improvement in the new fuzzing tests to ensure their correctness and completeness. Specifically, I've suggested adding a missing test case in BitShuffleFuzzer, correcting a logically flawed test case for CRC32C, and fixing incorrect ByteBuffer API usage in SnappyCombinedFuzzer.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces fuzzing targets for snappy-java, which is a great step towards improving the project's security and robustness. The new fuzzers cover various APIs including the raw API, framed streams, and BitShuffle. My review focuses on the structure and correctness of these new fuzzers. I've found some areas for improvement, mainly related to code redundancy and separation of concerns between the different fuzzers. Specifically, there's an overlap in testing the BitShuffle algorithm between SnappyCombinedFuzzer and the dedicated BitShuffleFuzzer. There are also some redundant test cases within the fuzzers that could be simplified. Addressing these points will make the new fuzzing suite more maintainable and efficient.
| public static void fuzzerTestOneInput(FuzzedDataProvider data) { | ||
| int selector = data.consumeInt(0, 7); | ||
| switch (selector) { | ||
| case 0: | ||
| testRawApi(data); | ||
| break; | ||
| case 1: | ||
| testFramed(data); | ||
| break; | ||
| case 2: | ||
| runFuzz(() -> testCrc32C(data)); | ||
| break; | ||
| case 3: | ||
| testBlockStream(data); | ||
| break; | ||
| case 4: | ||
| testUtil(data); | ||
| break; | ||
| case 5: | ||
| testBitShuffle(data); | ||
| break; | ||
| case 6: | ||
| testHadoopStream(data); | ||
| break; | ||
| case 7: | ||
| testByteBuffer(data); | ||
| break; | ||
| } |
There was a problem hiding this comment.
This fuzzer duplicates the functionality of BitShuffleFuzzer. The testBitShuffle method and parts of testUtil (cases 3 and 4) are for fuzzing BitShuffle, which is already covered by BitShuffleFuzzer.java. This contradicts the PR summary which states that BitShuffleFuzzer covers the BitShuffle algorithm.
To maintain a clear separation of concerns and avoid redundant test execution, all BitShuffle fuzzing logic should be confined to BitShuffleFuzzer.
I recommend the following changes:
- Remove the
testBitShufflemethod (lines 402-434) from this file. - Remove cases 3 and 4 from the
testUtilmethod (lines 379-398). - Update this
switchstatement to reflect these removals (e.g., removecase 5fortestBitShuffle, update range fortestUtil's switch, and update this switch's range).
| switch (data.consumeInt(0, 9)) { | ||
| case 0: | ||
| fuzzBitshuffle(data.consumeInts(SIZE), BitShuffle::shuffle, BitShuffle::unshuffleIntArray, "int[]"); | ||
| break; | ||
| case 1: | ||
| fuzzBitshuffle(data.consumeLongs(SIZE), BitShuffle::shuffle, BitShuffle::unshuffleLongArray, "long[]"); | ||
| break; | ||
| case 2: | ||
| fuzzBitshuffle(data.consumeShorts(SIZE), BitShuffle::shuffle, BitShuffle::unshuffleShortArray, "short[]"); | ||
| break; | ||
| case 3: | ||
| fuzzBitshuffle(data.consumeFloats(SIZE), BitShuffle::shuffle, BitShuffle::unshuffleFloatArray, "float[]"); | ||
| break; | ||
| case 4: | ||
| fuzzBitshuffle(data.consumeDoubles(SIZE), BitShuffle::shuffle, BitShuffle::unshuffleDoubleArray, "double[]"); | ||
| break; | ||
| case 5: | ||
| fuzzBitshuffle(new int[0], BitShuffle::shuffle, BitShuffle::unshuffleIntArray, "empty int[]"); | ||
| break; | ||
| case 6: | ||
| fuzzBitshuffle(new float[0], BitShuffle::shuffle, BitShuffle::unshuffleFloatArray, "empty float[]"); | ||
| break; | ||
| case 7: | ||
| fuzzBitshuffle(new double[0], BitShuffle::shuffle, BitShuffle::unshuffleDoubleArray, "empty double[]"); | ||
| break; | ||
| case 8: | ||
| fuzzBitshuffle(new short[0], BitShuffle::shuffle, BitShuffle::unshuffleShortArray, "empty short[]"); | ||
| break; | ||
| case 9: | ||
| fuzzBitshuffle(new long[0], BitShuffle::shuffle, BitShuffle::unshuffleLongArray, "empty long[]"); | ||
| break; | ||
| } |
There was a problem hiding this comment.
The test cases for empty arrays (5-9) are redundant. The consume...s(SIZE) methods used in cases 0-4 can return an array with up to SIZE elements, depending on the available fuzzing data. This includes the possibility of an empty array. Therefore, these explicit empty array tests are likely unnecessary as the fuzzer will explore empty inputs for the consume...s methods. You can simplify the code by removing cases 5-9 and adjusting the range of consumeInt.
switch (data.consumeInt(0, 4)) {
case 0:
fuzzBitshuffle(data.consumeInts(SIZE), BitShuffle::shuffle, BitShuffle::unshuffleIntArray, "int[]");
break;
case 1:
fuzzBitshuffle(data.consumeLongs(SIZE), BitShuffle::shuffle, BitShuffle::unshuffleLongArray, "long[]");
break;
case 2:
fuzzBitshuffle(data.consumeShorts(SIZE), BitShuffle::shuffle, BitShuffle::unshuffleShortArray, "short[]");
break;
case 3:
fuzzBitshuffle(data.consumeFloats(SIZE), BitShuffle::shuffle, BitShuffle::unshuffleFloatArray, "float[]");
break;
case 4:
fuzzBitshuffle(data.consumeDoubles(SIZE), BitShuffle::shuffle, BitShuffle::unshuffleDoubleArray, "double[]");
break;
}| testFramed(data); | ||
| break; | ||
| case 2: | ||
| runFuzz(() -> testCrc32C(data)); |
There was a problem hiding this comment.
The runFuzz wrapper here is redundant because the testCrc32C method already uses runFuzz internally for its test cases. You can simplify this by calling testCrc32C(data) directly.
| runFuzz(() -> testCrc32C(data)); | |
| testCrc32C(data); |
| case 7: | ||
| runFuzz(() -> { | ||
| byte[] input = new byte[0]; | ||
| byte[] compressed = Snappy.compress(input); | ||
| byte[] uncompressed = Snappy.uncompress(compressed); | ||
| if (!Arrays.equals(input, uncompressed)) { | ||
| throw new IllegalStateException("Empty array roundtrip failed"); | ||
| } | ||
| }); | ||
| break; | ||
| case 8: | ||
| runFuzz(() -> { | ||
| byte[] input = new byte[]{data.consumeByte()}; | ||
| byte[] compressed = Snappy.compress(input); | ||
| byte[] uncompressed = Snappy.uncompress(compressed); | ||
| if (!Arrays.equals(input, uncompressed)) { | ||
| throw new IllegalStateException("Single byte roundtrip failed"); | ||
| } | ||
| }); | ||
| break; | ||
| case 9: | ||
| runFuzz(() -> { | ||
| int size = data.consumeInt(4095, 4096); | ||
| byte[] input = data.consumeBytes(size); | ||
| byte[] compressed = Snappy.compress(input); | ||
| byte[] uncompressed = Snappy.uncompress(compressed); | ||
| if (!Arrays.equals(input, uncompressed)) { | ||
| throw new IllegalStateException("Max size roundtrip failed"); | ||
| } | ||
| }); | ||
| break; |
There was a problem hiding this comment.
Cases 7, 8, and 9 in testRawApi are redundant.
- Case 7 tests an empty byte array.
- Case 8 tests a single-byte array.
- Case 9 tests arrays of size 4095 or 4096.
All of these scenarios are already covered by case 0, which uses data.consumeBytes(data.consumeInt(0, 4096)). This allows the fuzzer to generate inputs of any size within that range, including 0, 1, 4095, and 4096. Relying on the fuzzer to explore these edge cases is more effective and keeps the code simpler.
I suggest removing cases 7, 8, and 9, and adjusting the switch range in testRawApi accordingly.
1 participant
With this PR we will be fuzzing targets to enable continuous security testing via CIFuzz and OSS-Fuzz.
Summary
Files Added
.github/workflows/cifuzz.yml
src/test/java/org/xerial/snappy/fuzz/SnappyCombinedFuzzer.java
src/test/java/org/xerial/snappy/fuzz/BitShuffleFuzzer.java
src/test/java/org/xerial/snappy/fuzz/SnappyStreamFuzzer.java
Key Points
Minimal footprint: Only 3 fuzzers added to keep the codebase lean.
No build changes: Works seamlessly with the existing sbt build system.
CIFuzz ready: Includes a dedicated GitHub Actions workflow for automated testing.
Broad coverage: The combined fuzzer alone covers 8 different APIs.
All fuzzers currently compile successfully and are ready for integration.